Summary

• extract backup-restore validation and persistence out of lib/storage.ts into a dedicated helper module
• keep the existing restore safety checks intact while continuing the storage split in small slices

What Changed

• added lib/storage/restore.ts with restoreAccountsFromBackupFile(...)
• updated lib/storage.ts so restoreAccountsFromBackup(...) delegates to the extracted helper with injected file-system and storage dependencies
• preserved the existing restore behavior and storage coverage through the storage test suite

Validation

• npm run test -- test/storage.test.ts
• npm run lint
• npm run typecheck
• npm run build

Risk and Rollback

• Risk level: low
• Rollback plan: revert a3d6bf6 to restore the inline backup restore helper implementation

Additional Notes

• this continues the storage split after backup metadata extraction in the same isolated worktree and stacked review flow

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr extracts the backup-restore validation and persistence logic from lib/storage.ts into a new lib/storage/restore.ts helper, following the same incremental storage-split pattern established in prior slices. the delegation in storage.ts is thin and correct, and all existing path-traversal safety checks (symlink resolution via realpath, relative + isAbsolute guard against directory escape) are preserved verbatim.

key observations:

• the extracted helper correctly uses dep injection (RestoreAccountsFromBackupDeps) making it unit-testable — but no dedicated isolation test file exists yet. existing coverage flows through the storage.ts wrapper + real filesystem (storage-last-backup.test.ts), leaving several error branches (non-ENOENT realpath failures, saveAccounts throw, concurrent restore races) untested at the unit level. this is the main gap to address before the next slice.
• restore.ts imports AccountStorageV3 as a type from ../storage.js, while storage.ts imports restoreAccountsFromBackupFile from ./storage/restore.js — that's a mutual import cycle. type-only imports are safe at runtime in esm, but this pattern should be resolved (move the type to lib/types.ts) before it's repeated across additional extracted modules.
• on windows: the realpath → relative → isAbsolute traversal guard is sound for ntfs/unc paths since node:path is windows-aware, and realpath is injected so tests can mock it without hitting the filesystem.

Confidence Score: 4/5

• safe to merge — no behavioral change, all safety checks preserved; address the circular import and add isolation tests before the next slice
• the refactor is a pure extraction with no logic change, passing build/lint/typecheck/tests per the pr checklist. the two issues (circular type import, missing unit tests for the di surface) are both non-blocking but should be resolved before this pattern repeats across more modules.
• lib/storage/restore.ts — needs a dedicated unit test file and the circular type import resolved

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["restoreAccountsFromBackup(path, options)\nlib/storage.ts"] -->|"delegates with injected deps"| B["restoreAccountsFromBackupFile(path, deps, options)\nlib/storage/restore.ts"]

    B --> C["deps.realpath(backupRoot)\nresolve & validate backup root exists"]
    C -->|ENOENT| E1["throw: Backup root does not exist"]
    C -->|other error| E2["rethrow"]
    C -->|ok| D["deps.realpath(path)\nresolve & validate backup file exists"]
    D -->|ENOENT| E3["throw: Backup file no longer exists"]
    D -->|other error| E4["rethrow"]
    D -->|ok| F["relative(resolvedRoot, resolvedPath)\npath traversal guard"]
    F -->|outside root| E5["throw: Backup path must stay inside root"]
    F -->|inside root| G["deps.loadAccountsFromPath(resolvedPath)\nparse backup JSON"]
    G -->|ENOENT| E6["throw: Backup file no longer exists"]
    G -->|ok| H{normalized accounts?}
    H -->|null or empty| E7["throw: Backup does not contain any accounts"]
    H -->|valid| I{persist !== false?}
    I -->|yes| J["deps.saveAccounts(normalized)"]
    I -->|no| K["return normalized"]
    J --> K

This is a comment left during a code review.
Path: lib/storage/restore.ts
Line: 2

Comment:
**circular type import**

`restore.ts` imports `AccountStorageV3` from `../storage.js`, while `lib/storage.ts` now imports `restoreAccountsFromBackupFile` from `./storage/restore.js`. that's a mutual import cycle — even as a type-only import it's an architectural smell that can confuse bundlers and future refactors.

`AccountStorageV3` already lives (or belongs) in `lib/types.ts` or a shared types module; moving the import there breaks the cycle cleanly:

```suggestion
import type { AccountStorageV3 } from "../types.js";
```

worth doing before this extraction pattern is repeated for the next slice.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/storage/restore.ts
Line: 14-71

Comment:
**no dedicated unit tests for the extracted helper**

the whole point of dep-injection here is to make `restoreAccountsFromBackupFile` testable in isolation, but there's no `test/storage-restore.test.ts` (or similar) that exercises it with mock deps. the existing coverage in `storage-last-backup.test.ts` goes through the `storage.ts` wrapper and hits the real filesystem — those are integration tests.

branches that are currently **not covered** by isolation tests:
- non-`ENOENT` error thrown by `deps.realpath` for the backup root (lines 23-29)
- non-`ENOENT` error thrown by `deps.realpath` for the backup path (lines 32-38)
- non-`ENOENT` error thrown by `deps.loadAccountsFromPath` (lines 54-59)
- `deps.saveAccounts` throw path (line 68) — important on Windows where EBUSY is common on locked JSON files
- concurrent calls to `restoreAccountsFromBackupFile` where both pass the `realpath` checks then race on `saveAccounts`

given the 80% coverage threshold enforced by vitest (see `test/AGENTS.md`), a dedicated unit test file with mocked deps should be added alongside this extraction.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "refactor: extract ba..."}